relay: add pullBinlogs interface for dm-worker to get local/grpc relay log #2216

lichunzhu · 2021-10-13T06:09:09Z

What problem does this PR solve?

Close #2214

What is changed and how it works?

Add PullBinlogs interface for server side.
Add PullBinlogs for client side for local way.

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change
Has exported variable/fields change

Related changes

Need to cherry-pick to the release branch

ti-chi-bot · 2021-10-13T06:09:10Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Ehco1996

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

dm/worker/subtask.go

lichunzhu · 2021-10-13T06:16:03Z

/cc @D3Hunter

ti-chi-bot · 2021-10-13T06:16:17Z

@lichunzhu: GitHub didn't allow me to request PR reviews from the following users: D3Hunter.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @D3Hunter

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dm/proto/dmworker.proto

dm/worker/server.go

dm/worker/subtask.go

lance6716

will review later

dm/proto/dmworker.proto

dm/worker/server.go

dm/worker/source_worker.go

D3Hunter · 2021-10-14T10:03:50Z

dm/worker/server.go

@@ -940,3 +940,42 @@ func (s *Server) GetWorkerCfg(ctx context.Context, req *pb.GetWorkerCfgRequest)
 	resp.Cfg, err = s.cfg.Toml()
 	return resp, err
 }
+
+// PullBinlogs will start a goroutine to continuously parse binlogs in relay dir and send them in streaming way.
+func (s *Server) PullBinlogs(req *pb.PullBinlogReq, stream pb.Worker_PullBinlogsServer) error {


this api's operating on SourceWorker layer
also we may have more than one SourceWorker later

maybe it (or part of the implementation of it) can move into SourceWorker layer, not Server layer

We will only register one grpc server service in dm-worker Server layer so I put this function here. How to register grpc server if we put this function to SourceWorker layer?

or part of the implementation of it, then server calls it, to separate the api and the work(belongs to SourceWorker i think)

D3Hunter · 2021-10-14T10:44:02Z

dm/proto/dmworker.proto

+
+message PullBinlogReq {
+    // Specifies which source of binlog to pull.
+    string source = 1;


what's the value of source? source-name or server-id?

there's another layer of sequence in relay dir, can it handle that?

<deploy_dir>/relay_log/ |-- 7e427cc0-091c-11e9-9e45-72b7c59d52d7.000001

Source is source-name. UUID is the detail that relay to think about. For the dm-worker layer I think we'd better use source.

Current relay can handle this so I think it's okay.

relay log filename maybe the same under different relay dir, for pos-based sync, it maybe ambiguous

<deploy_dir>/relay_log/ |-- 7e427cc0-091c-11e9-9e45-72b7c59d52d7.000001 --|_mysql-000001 |-- another-server-under-load-balancer.000001 --|_mysql-000001

I see what you mean. Relay replicated by binlog position shouldn't support switch MySQL leader/follower. So I think using Source doesn't have a problem.

…into addBinlogInterface

D3Hunter · 2021-10-15T02:44:16Z

dm/worker/server.go

+		return terror.ErrWorkerPullBinlogsInvalidRequest.Generate("worker doesn't enable relay")
+	}
+
+	ch, ech := s.worker.relayHolder.PullBinlogs(ctx, req)


s.worker -> w?

dm/proto/dmworker.proto

Ehco1996

rest LGTM

dm/proto/dmworker.proto

lichunzhu · 2021-10-19T08:55:39Z

ping @lance6716 @Ehco1996 @D3Hunter

Ehco1996

LGTM

D3Hunter · 2021-10-19T10:14:15Z

syncer/syncer.go

@@ -222,7 +223,8 @@ type Syncer struct {
 }

 // NewSyncer creates a new Syncer.
-func NewSyncer(cfg *config.SubTaskConfig, etcdClient *clientv3.Client) *Syncer {
+func NewSyncer(cfg *config.SubTaskConfig, etcdClient *clientv3.Client,
+	pullLocalBinlogs func(context.Context, *pb.PullBinlogReq) (chan *replication.BinlogEvent, chan error)) *Syncer {


why syncer needs to care about api PullBinlog?

because streamerController in sycner need to use this to pull and control binlog 🤔

In this PR, create reader is on relay and syncer need to access relay's createReader method.

even with that design, it would be better to abstract it, not passing a function(with full signature) around, it's hard to extend and read.

glorv · 2021-10-19T04:13:18Z

dm/proto/dmworker.proto

+    Location startFrom = 2;
+}
+
+message PullBinlogResp {


Do we need to include a error field here? What if the source is not exist in the target worker?

ti-chi-bot · 2021-10-26T10:48:38Z

@lichunzhu: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lichunzhu · 2021-10-27T09:37:02Z

merge this in next sprint

lance6716 · 2021-11-25T03:20:26Z

BTW, the perfect interface in my mind is using MySQL replication protocal, so someday we can decouple "relay log" functionalities from DM worker, DM worker will transparently connect to relay log nodes or real upstream.

cc @sunzhaoyang

lichunzhu added 2 commits October 13, 2021 10:23

finish server interface definition

153e733

add binlog reader method

a8d2cda

Merge branch 'master' into addBinlogInterface

ae2b20e

ti-chi-bot requested review from Ehco1996 and lance6716 October 13, 2021 06:09

ti-chi-bot added the size/XXL label Oct 13, 2021

lichunzhu commented Oct 13, 2021

View reviewed changes

dm/worker/subtask.go Show resolved Hide resolved

lichunzhu requested a review from glorv October 13, 2021 06:12

Ehco1996 reviewed Oct 13, 2021

View reviewed changes

dm/proto/dmworker.proto Outdated Show resolved Hide resolved

dm/worker/server.go Outdated Show resolved Hide resolved

dm/worker/server.go Outdated Show resolved Hide resolved

dm/worker/subtask.go Show resolved Hide resolved

ti-chi-bot added the needs-rebase label Oct 14, 2021

lance6716 reviewed Oct 14, 2021

View reviewed changes

dm/proto/dmworker.proto Outdated Show resolved Hide resolved

dm/proto/dmworker.proto Outdated Show resolved Hide resolved

dm/worker/server.go Outdated Show resolved Hide resolved

dm/worker/source_worker.go Outdated Show resolved Hide resolved

D3Hunter reviewed Oct 14, 2021

View reviewed changes

lichunzhu added 3 commits October 14, 2021 20:40

address comments

910dc42

Merge branch 'addBinlogInterface' of https://github.com/lichunzhu/dm …

0d37881

…into addBinlogInterface

merge master and resolve conflicts

b21aacb

ti-chi-bot removed the needs-rebase label Oct 14, 2021

Ehco1996 added this to the v2.1.0 milestone Oct 14, 2021

D3Hunter reviewed Oct 15, 2021

View reviewed changes

lance6716 reviewed Oct 15, 2021

View reviewed changes

dm/proto/dmworker.proto Show resolved Hide resolved

dm/proto/dmworker.proto Show resolved Hide resolved

lichunzhu added 2 commits October 15, 2021 14:11

address comments

6960a06

fix hound

516f902

Ehco1996 reviewed Oct 19, 2021

View reviewed changes

dm/proto/dmworker.proto Show resolved Hide resolved

lichunzhu added 2 commits October 19, 2021 16:49

address comment

9430889

resolve conflicts

8fad379

Ehco1996 approved these changes Oct 19, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 One reviewer already commented LGTM label Oct 19, 2021

D3Hunter reviewed Oct 19, 2021

View reviewed changes

glorv reviewed Oct 20, 2021

View reviewed changes

Ehco1996 mentioned this pull request Oct 21, 2021

relay/syncer: relay notifies syncer of new write to reduce sync latency #2225

Merged

ti-chi-bot added the needs-rebase label Oct 26, 2021

lichunzhu removed this from the v2.1.0 milestone Oct 27, 2021

lichunzhu closed this Nov 25, 2021

lichunzhu deleted the addBinlogInterface branch November 25, 2021 03:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

relay: add pullBinlogs interface for dm-worker to get local/grpc relay log #2216

relay: add pullBinlogs interface for dm-worker to get local/grpc relay log #2216

lichunzhu commented Oct 13, 2021

ti-chi-bot commented Oct 13, 2021 •

edited

Loading

lichunzhu commented Oct 13, 2021

ti-chi-bot commented Oct 13, 2021

lance6716 left a comment

D3Hunter Oct 14, 2021 •

edited

Loading

lichunzhu Oct 14, 2021

D3Hunter Oct 15, 2021 •

edited

Loading

D3Hunter Oct 14, 2021

lichunzhu Oct 14, 2021

D3Hunter Oct 15, 2021

lichunzhu Oct 15, 2021 •

edited

Loading

D3Hunter Oct 15, 2021

Ehco1996 left a comment

lichunzhu commented Oct 19, 2021

Ehco1996 left a comment

D3Hunter Oct 19, 2021

Ehco1996 Oct 20, 2021 •

edited

Loading

lichunzhu Oct 20, 2021

D3Hunter Oct 20, 2021 •

edited

Loading

glorv Oct 19, 2021

ti-chi-bot commented Oct 26, 2021

lichunzhu commented Oct 27, 2021

lance6716 commented Nov 25, 2021

relay: add pullBinlogs interface for dm-worker to get local/grpc relay log #2216

relay: add pullBinlogs interface for dm-worker to get local/grpc relay log #2216

Conversation

lichunzhu commented Oct 13, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

ti-chi-bot commented Oct 13, 2021 • edited Loading

lichunzhu commented Oct 13, 2021

ti-chi-bot commented Oct 13, 2021

lance6716 left a comment

Choose a reason for hiding this comment

D3Hunter Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D3Hunter Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lichunzhu Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ehco1996 left a comment

Choose a reason for hiding this comment

lichunzhu commented Oct 19, 2021

Ehco1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ehco1996 Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D3Hunter Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot commented Oct 26, 2021

lichunzhu commented Oct 27, 2021

lance6716 commented Nov 25, 2021

ti-chi-bot commented Oct 13, 2021 •

edited

Loading

D3Hunter Oct 14, 2021 •

edited

Loading

D3Hunter Oct 15, 2021 •

edited

Loading

lichunzhu Oct 15, 2021 •

edited

Loading

Ehco1996 Oct 20, 2021 •

edited

Loading

D3Hunter Oct 20, 2021 •

edited

Loading